Fix self-recursive action return type inference (issue #590) - #591
Conversation
… type-checks (#590) A self-recursive action that used its own recursive result inside its body (e.g. indexed it) got a false `Cannot index into Nothing` diagnostic. The body is type-checked before the real return type is inferred (#575's ordering), and the provisional return type was seeded as `Nothing`, so a self-reference in the body resolved to `Nothing` and any use/indexing of it raised strict "found Nothing" errors. Seed the provisional return type as `Unknown` instead. After #588/#589 an `Unknown`-typed value degrades gracefully, so self-references resolve cleanly during the body check while post-body inference (#575) still records the concrete return type for external callers. Void actions are still recorded as `Nothing` externally, preserving existing behavior. Adds regression tests covering the reported repro and the Scribe `scribe_p_unary` shape that negates its recursive result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Qykg1eQ2bJKx2uoJNBGPj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe type checker's handling of unannotated action return types was changed to seed with ChangesTypechecker return type inference fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant TypeChecker
participant ActionSymbol
participant BodyChecker
Parser->>TypeChecker: Statement::ActionDefinition (no return_type)
TypeChecker->>ActionSymbol: seed Type::Function { return_type: Unknown }
TypeChecker->>BodyChecker: check_statement_types(body)
BodyChecker->>ActionSymbol: self-recursive call sees Unknown (no Nothing error)
BodyChecker-->>TypeChecker: inferred return type from return statements
TypeChecker->>ActionSymbol: update Type::Function { return_type: inferred }
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/recursive_action_return_type_test.rs (1)
19-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating near-duplicate tests and strengthening assertions.
The two tests share nearly identical WFL source and structure, differing only in whether the recursive result is negated before indexing. Extracting a shared helper (parameterized by the small code difference) would reduce duplication. Separately, both tests only assert on the specific "Cannot index into Nothing" substring when
resultisErr— if type-checking unexpectedly starts failing for a different reason, or if it now spuriously succeeds without exercising the intended path, the test won't catch it. Consider assertingresult.is_ok()(or a more specific check) in addition to the negative substring check, to make the regression guard tighter.Also applies to: 59-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/recursive_action_return_type_test.rs` around lines 19 - 54, The recursive action type-check tests are duplicated and the current assertion only guards against one error substring, so strengthen the regression check. In test_self_recursive_action_result_not_typed_nothing and the related recursive test in recursive_action_return_type_test.rs, extract the shared WFL setup into a helper parameterized by the small expression difference, then assert the intended outcome directly with result.is_ok() (or an equivalent explicit success check) before keeping the negative “Cannot index into Nothing” guard. Use the existing Parser, TypeChecker, and lex_wfl_with_positions flow to keep the test focused on recursive return typing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/recursive_action_return_type_test.rs`:
- Around line 19-54: The recursive action type-check tests are duplicated and
the current assertion only guards against one error substring, so strengthen the
regression check. In test_self_recursive_action_result_not_typed_nothing and the
related recursive test in recursive_action_return_type_test.rs, extract the
shared WFL setup into a helper parameterized by the small expression difference,
then assert the intended outcome directly with result.is_ok() (or an equivalent
explicit success check) before keeping the negative “Cannot index into Nothing”
guard. Use the existing Parser, TypeChecker, and lex_wfl_with_positions flow to
keep the test focused on recursive return typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f637b843-1f2a-4f69-b8d5-e0e3d8470997
📒 Files selected for processing (2)
src/typechecker/mod.rstests/recursive_action_return_type_test.rs
…tion (#590) Address review feedback on PR #591: extract the shared lex/parse/typecheck flow into `assert_typechecks_clean`, and assert the programs type-check with zero diagnostics (`result.is_ok()`) instead of only checking for the absence of one error substring. The tighter guard catches both a re-introduced "Cannot index into Nothing" error and any new spurious diagnostic on the recursive path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Qykg1eQ2bJKx2uoJNBGPj
…#599) * fix: infer action return types through try blocks and for container methods (#560) Two residual shapes of issue #560 still produced false 'Cannot index into Nothing' diagnostics after #575/#591: - collect_return_types never descended into try statements, so an action whose only returns live inside a try body, when-error clause, otherwise, or finally block was inferred as returning Nothing. It now traverses TryStatement and WaitForStatement (check_return_statements kept in sync). - Container methods were registered with return_type Nothing when unannotated and never refined, so instance.method() results hit the same false error. The analyzer now seeds unannotated methods with a provisional Unknown, and the type checker infers the real return type from each method body (parameters in scope, mirroring the top-level action arm) and writes it back to the container registry via a new Analyzer::get_container_mut. Inherited method calls read the same registry entries, so they are fixed too. Static-diagnostics-only change; runtime behavior is unchanged. TDD: tests/action_return_type_residuals_test.rs was confirmed failing (4/4) before the fix and passes after, alongside the full test suite and all TestPrograms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k * fix: refine static container method return types and validate annotated method returns Address CodeRabbit review on #599: - Static methods were seeded with the provisional Unknown but never refined: the ContainerDefinition arm only iterated instance methods. Value-returning statics stayed Unknown forever and void statics lost their previous Nothing type. Static methods now go through the same body-check + infer + write-back loop, updating container_info.static_methods. (Static method calls remain a runtime future feature; the registry refinement keeps Container.method member access accurate and restores Nothing for void statics.) - Annotated container methods (action name: Type) now have their return statements validated against the annotation via check_return_statements, mirroring the top-level action arm. - Added a registry-level unit test pinning both static cases (inferred List for a value-returning static, Nothing for a void static), since a typecheck-clean integration test cannot observe static calls that the runtime rejects. Dev diary updated to match the implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k * chore: remove stray test artifacts accidentally committed flush_test_*.txt, test_output.txt, and a google_index.html overwrite were produced by running the TestPrograms suite locally and swept in by git add -A. Remove the artifacts and restore google_index.html to its prior content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Fixes a type-checking regression where self-recursive actions that use their own recursive result (e.g., indexing it) would incorrectly raise "Cannot index into Nothing" diagnostics. The issue occurred because the provisional return type was seeded as
Nothing, causing self-references in the action body to resolve againstNothingbefore the real return type was inferred.Changes
Seed provisional return type as
Unknowninstead ofNothing: When type-checking an action body (which happens before return-type inference per fix: infer user-defined action return types in the type checker #575), self-recursive calls now resolve againstUnknownrather thanNothing. This allows graceful degradation after Type checker:store x as <action call>raises ERROR "Could not infer type for variable" whenever the callee's return type is Unknown #588/fix: bind Unknown-typed store results silently under gradual typing (#588) #589 while avoiding strict "found Nothing" errors.Preserve void action behavior: The fix ensures that void actions (those with pure
Nothingreturn types) still reportNothingto external callers, maintaining backward compatibility. TheUnknownseed is only used internally during body type-checking.Updated comment documentation: Clarified the rationale for seeding a provisional type and explained why
Unknownis preferred overNothingfor self-references.Implementation Details
Unknown-typed values degrade gracefully in type operations (per Type checker:store x as <action call>raises ERROR "Could not infer type for variable" whenever the callee's return type is Unknown #588/fix: bind Unknown-typed store results silently under gradual typing (#588) #589), so indexing and other operations on self-recursive results now type-check cleanly.Unknownseed is internal to the body-checking phase.Tests
Added two regression tests in
tests/recursive_action_return_type_test.rs:test_self_recursive_action_result_not_typed_nothing: Verifies that indexing a self-recursive action's result does not raise false "Cannot index into Nothing" errors.test_self_recursive_action_negating_result_typechecks_clean: Ensures self-recursive actions that negate their recursive result also type-check cleanly.https://claude.ai/code/session_018Qykg1eQ2bJKx2uoJNBGPj
Summary by CodeRabbit